Use pre-built timezone/directory maps by default. #193
Conversation
|
Compile times are down, but are still quite high. I think generating a binary encoded blob for the timespan information and attaching it with |
orlp
changed the title
|
Have you already looked at #165? Not sure if your ideas are aligned with that (or are complimentary). |
|
@djc The idea is complimentary. You could choose to have such an encoding in the binary blob. The PRs would invalidate each other though. |
chrono-tz/build.rs
Outdated
| #[cfg(not(feature = "filter-by-regex"))] | ||
| { | ||
| if std::env::var("CHRONO_TZ_TIMEZONE_FILTER").is_ok() { | ||
| println!("cargo:warning=CHRONO_TZ_TIMEZONE_FILTER set without enabling filter-by-regex feature") |
There was a problem hiding this comment.
From the docs:
The warning instruction tells Cargo to display a warning after the build script has finished running. Warnings are only shown for path dependencies (that is, those you’re working on locally), so for example warnings printed out in crates.io crates are not emitted by default, unless the build fails.
Not sure this is useful?
(Also the docs seem to indicate it should be cargo::warning, with two colons.
There was a problem hiding this comment.
Hmm, maybe we also need to fail the build in that case, in addition to the warning. My hope was to give a simple warning without a hard failure.
I tried using cargo::warning but apparently that is new syntax. The single-colon syntax is required for the MSRV of chrono-tz.
There was a problem hiding this comment.
I've switched this now to use cargo::error which is only supported on newer versions. So either the library is larger than expected but still works on older versions, or you get a helpful error telling you to enable the feature.
chrono-tz/src/directory.rs
Outdated
| #[cfg(not(feature = "filter-by-regex"))] | ||
| include!("prebuilt_directory.rs"); |
There was a problem hiding this comment.
Could we instead have #[path = "prebuilt_directory.rs"] on the mod declaration in lib.rs conditional on a cfg(), or does that not work? Otherwise, can we use a mod declaration here and re-export all symbols? I'd like the prebuilt modules to be visible to Rust-Analyzer et al.
There was a problem hiding this comment.
I think that'd work (the first suggestion), I'll try it.
There was a problem hiding this comment.
Unfortunately I can't get this to work, because we need the OUT_DIR environment variable but the path attribute only accepts string literals.
There was a problem hiding this comment.
For what it's worth rust-analyzer seems to be able to pierce through the include! barrier.
chrono-tz-build/src/lib.rs
Outdated
| let src_dir = env::current_dir().unwrap(); | ||
| let timezone_path = src_dir.join("src").join("prebuilt_timezones.rs"); | ||
| let mut timezone_file = File::create(timezone_path).unwrap(); | ||
| write_timezone_file(&mut timezone_file, &table).unwrap(); | ||
|
|
||
| let directory_path = src_dir.join("src").join("prebuilt_directory.rs"); | ||
| let mut directory_file = File::create(directory_path).unwrap(); | ||
| write_directory_file(&mut directory_file, &table, &iana_db_version).unwrap(); |
There was a problem hiding this comment.
Can we extract the duplicated code out into a function instead?
|
@djc Could you take another look? |
Resolves #192.
Now warns you if you have
CHRONO_TZ_TIMEZONE_FILTERset but haven't enabled thefilter-by-regexfeature.If the timezone information is updated one must run
before publishing the crate. You may want to put this in some form of CI to ensure this happens.